Skip to content

Run the Security plugin under FIPS approved-only mode with BC providers - #5866

Open
beanuwave wants to merge 10 commits into
opensearch-project:mainfrom
sternadsoftware:fips_compliance4
Open

Run the Security plugin under FIPS approved-only mode with BC providers#5866
beanuwave wants to merge 10 commits into
opensearch-project:mainfrom
sternadsoftware:fips_compliance4

Conversation

@beanuwave

@beanuwave beanuwave commented Dec 18, 2025

Copy link
Copy Markdown
Contributor

Description

Category: Enhancement, New feature, Bug fix

This branch makes the Security plugin run correctly under FIPS approved-only mode (BCFIPS as the sole crypto provider). Every change traces to one of: an algorithm/format/usage that FIPS disallows (whether or not BCFIPS rejects it at runtime), a weakness surfaced during the audit, or build/packaging plumbing to make FIPS the always-available baseline.

At a glance:

  • Single source of truth: FipsMode.isEnabled() (env OPENSEARCH_FIPS_MODE=true) replaces CryptoServicesRegistrar.isInApprovedOnlyMode() everywhere. Intent is decoupled from provider state and cross-checked at startup.
  • Build & FIPS activation: BCFIPS is always built (compileOnly, shipped by core); the compile-time FipsBuildParams fork is gone. FIPS engages purely through the java.security the JVM loads (no provider is registered in code) - the runtime launcher merges in fips_java.security on OPENSEARCH_FIPS_MODE=true, while tests swap it wholesale (-D...properties==<file>).
  • OBO/JWT tokens: encryption rewritten from default-mode AES (ECB) -> AES-GCM + HKDF (fixes a FIPS/NIST violation and a deterministic-crypto weakness), plus an entropy floor, lazy fail-closed init, an optional BCFKS keystore for the keys, and secret redaction.
  • Keystores / TLS: JKS/PKCS12 out, BCFKS in (PKCS11 opt-in, signed via SunJSSE); PemKeyReader rebuilt on BouncyCastle; TLSv1.1 dropped in FIPS mode.
  • LDAP: SNI re-implemented for BCTLS, and ldaptive's non-FIPS internal PKCS12 key-copy bypassed.
  • Auth hardening: Kerberos cleanup, timing-safe dummy hash, a 14-char password floor in FIPS, approved-DRBG randomness, key-material zeroing.
  • CLI: securityadmin accepts BCFKS/PKCS11 and now launches via core's opensearch-cli.

Key changes

OBO / JWT tokens

The OBO key path was audited end-to-end; the encryption rewrite is the FIPS trigger, the rest are weaknesses found alongside it.

  • Encryption rewrite. EncryptionDecryptionUtil: built its cipher with the bare Cipher.getInstance("AES") and a key forced to 16 bytes via Arrays.copyOf (silently truncating or zero-padding) -> now AES-256-GCM (random 12-byte IV, 128-bit tag), key derived via HKDF-SHA256. A bare "AES" transformation resolves to the JCE provider's default mode - ECB - which for data confidentiality violates NIST SP 800-38A2 (ECB is approved only for key wrapping, SP 800-38F3). The mode is never spelled out or configurable, and BC FIPS permits the ECB primitive at runtime - so this had to be caught by review, not the provider.
  • Entropy floor (SP 800-133r25). HKDF can't create entropy, so a short encryption_key would produce a nominal AES-256 key with sub-112-bit strength. A FIPS guard rejects input keying material < 32 bytes and zeroes the IKM after derivation.
  • Signing-key length check. Validates the decoded key bytes; measuring the Base64 string length would over-count by ~4/3 and let a 384-bit key pass a 512-bit gate.
  • Lazy, fail-closed init. OnBehalfOfAuthenticator initializes lazily and atomically: the constructor is inert, the first token request triggers init, a failure logs once and declines - a bad key can't throw out of the constructor, propagate through config reload, and retry forever. Mirrors ApiTokenAuthenticator.
  • Keystore support. The OBO signing and encryption keys can now be loaded from a BCFKS keystore (KeyUtils.loadKeyFromKeystore) instead of inline config, keeping the secrets out of cluster state. Relative *_keystore_path values resolve against the node config dir (consistent with other security file settings). This is an SP 800-576 key-at-rest hardening (protecting keying material / limiting exposure), not a FIPS 140-31 requirement.
  • Secret redaction (CWE-532). OnBehalfOfSettings.toString() redacts the signing_key/encryption_key values so they no longer leak into logs.

Keystores / TLS

  • PemKeyReader rewritten onto BouncyCastle (PEMParser / JcaPEMKeyConverter / PKCS8 decryptor) instead of raw JCE; adds BCFKS + PKCS11 and store-type auto-detection.
  • SSLConfigConstants: both defaults are now FIPS-conditional - default store type is forced to BCFKS in FIPS, and ALLOWED_SSL_PROTOCOLS drops TLSv1.1 in FIPS. Both LDAP backends now reuse ALLOWED_SSL_PROTOCOLS for their default enabled_ssl_protocols.
  • PKCS#11 keys are signed via SunJSSE, not BCJSSE. A PKCS#11/HSM private key is non-exportable and BCJSSE can't sign with it (no encoding for key); SunJSSE delegates the handshake signature to the key's own provider (SunPKCS11). So when the keystore is PKCS#11, SslConfiguration builds the SSLContext against SunJSSE (SslContextBuilder.sslContextProvider(SunJSSE)).
  • The BC FIPS provider is declared, not instantiated. main self-registered it at plugin load (OpenSearchSecuritySSLPlugin.tryAddSecurityProvider() -> Security.addProvider(new BouncyCastleFipsProvider())); that method is removed. Providers now come solely from the active java.security file (JCA lazy-loads them), so FIPS vs non-FIPS is a launch-time provider swap (BCJSSE vs SunJSSE) with no code branch - the security files are a core/distribution concern.

LDAP

  • SNI for BCTLS - two sequential (not duplicate) concerns: SNISettingTLSSocketFactory sets the ClientHello SNI before the handshake so a multi-cert server serves the right cert; HostnameVerifyingTrustManager checks the returned cert after. For IP targets the SNI factory early-returns (no SNI) and the trust manager is the only hostname check; for DNS it sets SNI + endpointIdentification (the trust-manager hostname check is then redundant). HostnameAwareConnectionFactory threads the target hostname through so SNI works.
  • Avoiding ldaptive's internal PKCS12 copy (both authentication and authorization paths) - ldaptive's create*CredentialConfig with key aliases routes through KeyStoreSSLContextInitializer.getKeyManagers(), which copies the private key into a fresh in-memory PKCS12 store; SunJCE protects it with PBEWithHmacSHA256AndAES_256, not available in FIPS. Fix: build BCFKS keystores from PEM via PemKeyReader.toTruststore/toKeystore and pass null key aliases, so kmf.init(keystore, password) is called directly and the PKCS12-copy branch is bypassed. (LDAPAuthorizationBackend previously used createX509CredentialConfig, which hit the same path unconditionally.)
  • Extracted the default backend's Java9CL into a shared SocketFactoryClassLoader (resolves the JNDI socket-factory classes by name) and set it on ldap2's JNDI provider config, so PrivilegedProvider's thread-context swap resolves the socket factory on reconnect. Fixes an ldap2 LDAPS reconnect ClassNotFoundException.

Auth hardening (found mid-audit)

  • HTTPSpnegoAuthenticator: no longer mutates global System.setProperty debug flags; stops logging the acceptor principal; proper LoginContext.logout() + decoded-header zeroing in finally.
  • InternalAuthenticationBackend + PasswordHasher.getDummyHash(): not-found timing path now uses the configured hasher (closes user-enumeration side-channel under PBKDF2).
  • Password-length floor. PBKDF2 keys are derived from the password itself, and BC FIPS rejects key material under 112 bits (< 14 ASCII chars) at hashing time. PasswordValidator.FIPS_MIN_PASSWORD_LENGTH (14) now anchors both ends: FIPS raises an unset restapi.password_min_length to 14, and startup rejects a lower explicit value - otherwise the REST API accepts passwords the hasher then refuses.
  • Randomness from core. Randomness.createSecure() replaces new SecureRandom() for OBO encryption, api-tokens, and user passwords - it resolves to the approved SP 800-90A4 DRBG in FIPS. UserService generates 20-27 chars in FIPS (>=119 bits over the 62-char alphabet), 8-15 otherwise; char[] zeroed in finally.

CLI

  • SecurityAdmin: accepts BCFKS/PKCS11; PKCS#11 PIN prompt. Launcher scripts now delegate to core's shared opensearch-cli. Standalone bundle ships BCFIPS jars under deps/.
  • SecurityAdmin.buildPkcs11SslContext routes PKCS#11 keys through SunJSSE client-side (same as the server TLS layer - see Keystores/TLS).

Note

Reviewer call-outs

  1. OBO token wire-format break - GCM-encrypted tokens are not interchangeable with old AES-ECB tokens across the upgrade boundary. Benign in practice (OBO TTL < 10 min, self-heals shortly after rollout completes), but worth a release note for hot/rolling deployments.
  2. securityadmin now launches via core's opensearch-cli - this targets the in-distribution path; the standalone bundle is no longer self-launching and is effectively deprecated.
  3. Test strategy diverges from core's convention - this branch runs one dual-mode suite (skip incompatible cases at runtime), whereas the core project forks the suite: a FIPS test class extends the non-FIPS one.
  4. Dropped the auto System.setProperty(disableEndpointIdentification, true) from the default ldap backend - now warn-only. Two reasons: (a) it aligns both backends on the same logic (ldap2 never set it); (b) mutating a global JVM property from application code is bad practice - it's process-wide and load-order-dependent, so one auth-domain's verify_hostnames: false silently reconfigured hostname checking for the whole JVM. Operators who need it must now set the -D flag deliberately.
  5. Why are verify_hostnames and trust_all coupled to the same verifier? - verifyHostnames = !trustAll && <setting>, so trust_all: true forces AllowAnyHostnameVerifier on top of AllowAnyTrustManager. Chain validation and hostname matching are orthogonal concerns; collapsing them onto one code path means you can't relax one without the other and hides which layer a config change actually touches. Worth untangling so each parameter maps to exactly one verification layer.

Core / distribution follow-ups

Not plugin changes - each fix belongs in core or the distribution, tracked here so it can be routed:

  • bctls-fips has no socket connect grant. Core's base security.policy grants bc-fips/bcpkix-fips but not bctls-fips, so under the agent all outbound BCJSSE TLS (LDAPS, audit sinks, remote reindex/snapshot over https) is denied - and plugin policies can't grant a core lib/ jar. Verified against a live LDAPS-over-FIPS cluster.
  • JUL->log4j bridge not active for BC FIPS at rollout. LogConfigurator sets java.util.logging.manager at runtime, too late for the BouncyCastle FIPS JSSE provider (which initialises JUL during bootstrap); its per-handshake INFO traces then leak to System.err as [WARN][stderr] spam. Fix in the distribution: set -Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager as a launch-time -D in jvm.options, and default org.bouncycastle.jsse to warn in the shipped log4j2.properties.
  • Core's org.opensearch.fips.FipsMode still detects FIPS via CryptoServicesRegistrar.isInApprovedOnlyMode() - the provider-state probe this branch replaced with the env var; it should follow.
  • BCFipsEntropyDaemonFilter should move into core's test framework. The local thread-filter accommodation for the "BC FIPS Entropy Daemon" thread belongs in core's BouncyCastleThreadFilter, not this plugin's test sources.

Additional notes

  • JKS can't hold SecretKey entries (engineSetKeyEntry requires PrivateKey) - relevant to the JWT-signing-key path; BCFKS is the only FIPS-approved store that holds secret keys.
  • Provider list is a complete override; SunJGSS is deliberately retained for Kerberos/SPNEGO.
  • Operational: keystore passwords must be >=14 chars (BCFIPS); user passwords likewise (the 112-bit PBKDF2 floor, now enforced - see Auth hardening); the "BC FIPS Entropy Daemon" thread required a test thread-filter accommodation.
  • SAML is scoped out - both SAML stacks it uses (OneLogin java-saml and OpenSAML/Shibboleth) are not FIPS-compliant (tests skip in FIPS mode).
  • HTTP/3 exclusion is a build-level constraint, not a categorical ban - a FIPS-certified BoringSSL substituted at the OS level re-enables it.
  • The SNI plumbing (SNISettingTLSSocketFactory + the SniAwareConnection decorator + hostname ThreadLocal + HostnameAwareConnectionFactory) works around the JNDI LDAP provider resolving hostnames to IPs before socket creation (bcgit/bc-java#460); ldaptive 2.x's native (Netty) transport opens sockets with the real hostname, which would let this entire stack be deleted (out of scope here).

Testing

The suite runs in non-FIPS mode by default. To exercise the FIPS code paths, set the environment variable before invoking Gradle:

OPENSEARCH_FIPS_MODE=true ./gradlew test integrationTest

When set, the build swaps in the FIPS java.security policy (BCFIPS-only providers), enables -Dorg.bouncycastle.fips.approved_only=true, and points the JVM at the BCFKS truststore. FIPS-incompatible tests (BCrypt, Argon2, SAML, SSLv3, JKS/PKCS12, weak/short passwords) are auto-skipped via JUnit assumptions. Static bcrypt fixtures and their short demo passwords are rewritten to PBKDF2 and padded past the 14-char floor by FipsHashAdapter (a no-op outside FIPS), and a few timing-sensitive integ tests scale down under FIPS, where PBKDF2 logins and BCTLS handshakes are markedly slower.

For a running cluster, select the FIPS-approved password hasher in opensearch.yml (BCrypt/Argon2 are not available in approved-only mode):

plugins.security.password.hashing.algorithm: pbkdf2

The demo hashes in config/opensearch-security/internal_users.yml are BCrypt, which won't verify under PBKDF2 - regenerate the hash for each test account (e.g. with tools/hash.sh) and replace it before applying the security config.

Test securityadmin.sh with BCFKS + PKCS#11 keystores (SoftHSM)

Exercises a token-resident node TLS key (signed via SunJSSE) and securityadmin authenticating with a PKCS#11 client key. This example is FIPS-specific, but adjusts easily to non-FIPS by registering a SunPKCS11 provider via OPENSEARCH_JAVA_OPTS="-Djava.security.properties=$OPENSEARCH_HOME/config/java.security" instead of OPENSEARCH_FIPS_MODE=true. All paths below are relative to $OPENSEARCH_HOME.

# 1. Set up a test cluster.
...
cd $OPENSEARCH_HOME
sh bin/opensearch-keystore create --password
sh plugins/opensearch-security/tools/install_demo_configuration.sh -y -i -s

# 2. Init token - the --pin becomes the keystore/truststore password.
softhsm2-util --init-token \
  --free \
  --label opensearch \
  --so-pin 4321 \
  --pin 1234

# 3. Register the provider in config/fips_java.security:
      security.provider.<n>=SunPKCS11 /path/to/config/softhsm-pkcs11.cfg
#    with softhsm-pkcs11.cfg in the same dir:
      name = SoftHSM
      library = /usr/lib/softhsm/libsofthsm2.so
      slotListIndex = 0

# 4. Import node + admin keys WITH chains (PKCS#12 -> token).
#    Repeat the keytool step for kirk (swap -name / -srcalias / -destalias).
openssl pkcs12 -export \
  -inkey config/esnode-key.pem \
  -in config/esnode.pem \
  -certfile config/root-ca.pem \
  -name esnode-cert \
  -out /tmp/esnode.p12 \
  -passout pass:1234

jdk/bin/keytool \
  -importkeystore \
  -srckeystore /tmp/esnode.p12 \
  -srcstoretype PKCS12 \
  -srcstorepass 1234 \
  -srcalias esnode-cert \
  -destkeystore NONE \
  -deststoretype PKCS11 \
  -deststorepass 1234 \
  -destalias esnode-cert \
  -addprovider SunPKCS11 \
  -providerarg config/softhsm-pkcs11.cfg

# 5. Trust anchor -> BCFKS file.
jdk/bin/keytool -importcert -noprompt \
  -alias root-ca \
  -file config/root-ca.pem \
  -keystore config/root-ca.bcfks \
  -storetype BCFKS \
  -storepass changeit \
  -providerClass org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider \
  -providerPath lib/bc-fips-2.1.2.jar

# 6. In opensearch.yml: comment the demo *.pem*_filepath lines, keep admin_dn: CN=kirk,...,
#    and add for both transport and http:

# --- Node identity from the PKCS#11 token ---
plugins.security.ssl.transport.keystore_type: PKCS11
plugins.security.ssl.transport.keystore_alias: esnode-cert
plugins.security.ssl.transport.keystore_password: "1234"      # SoftHSM PIN
plugins.security.ssl.http.keystore_type: PKCS11
plugins.security.ssl.http.keystore_alias: esnode-cert
plugins.security.ssl.http.keystore_password: "1234"

# --- Trust anchor from a BCFKS file ---
plugins.security.ssl.transport.truststore_type: BCFKS
plugins.security.ssl.transport.truststore_filepath: root-ca.bcfks
plugins.security.ssl.transport.truststore_password: "changeit"
plugins.security.ssl.http.truststore_type: BCFKS
plugins.security.ssl.http.truststore_filepath: root-ca.bcfks
plugins.security.ssl.http.truststore_password: "changeit"

# 7. Run the cluster and apply security config with the PKCS#11 admin key.
#    Pass = 'Connected as "CN=kirk,..."' followed by 'Done with success'.
OPENSEARCH_FIPS_MODE=true sh plugins/opensearch-security/tools/securityadmin.sh \
  -cd config/opensearch-security/ \
  -icl \
  -nhnv \
  -cacert config/root-ca.pem \
  -kst PKCS11 \
  -kspass 1234 \
  -ksalias kirk
Test LDAP authentication over LDAPS (SNI, hostname verification, mTLS)

Self-contained manual tests for the LDAP TLS changes: SNI / hostname verification, mutual TLS, and the TLS protocol floor. Authentication only - these changes don't touch authz code, so role resolution is out of scope (verify that against a real directory).

Run them against any LDAPS directory that supports mTLS (a client cert is required). The walkthrough uses a local UnboundID in-memory stand-in only because it's repeatable and trivial to set up - a convenience, not a requirement; substitute your own server anywhere it appears. Its setup lives in LDAP_UNBOUNDID_STANDIN_GUIDE.md.

TLS material is the OpenSearch install's own demo certs in $OPENSEARCH_HOME/config/ - esnode (server, SAN includes localhost), root-ca.pem (trust anchor), kirk (client). Run the node with OPENSEARCH_FIPS_MODE=true (omit for non-FIPS; the only observable difference is the TLS protocol floor).

FIPS agent-build gotchas (both are Core / distribution follow-ups): (1) core's base security.policy grants bc-fips/bcpkix-fips but not bctls-fips, so every LDAPS bind is denied under the agent; (2) the JUL->log4j bridge isn't active for BC FIPS JSSE at handshake time, so each bind's INFO traces leak to stderr as [WARN][stderr] spam (noise, not a failure).

cd $OPENSEARCH_HOME
export LDAP_USER="testuser"; export LDAP_PASS="testpassword"   # cn=Test User,ou=people,o=TEST

# Optional trace logging for the "what to look for" lines (config/log4j2.properties):
#   logger.ldap.name=org.opensearch.security.auth.ldap
#   logger.ldap.level=trace
#   logger.ldap2.name=org.opensearch.security.auth.ldap2
#   logger.ldap2.level=trace
#   logger.ldaptive.name=org.ldaptive
#   logger.ldaptive.level=debug

# === Baseline config (authc.ldap.authentication_backend.config; o=TEST, no authz block) ======
# Edit this first, then apply below. Bare *_filepath resolve against config/; kirk + root-ca are the
# shipped demo certs, so as shipped this IS scenario 1a / 2a -> 200. Run every scenario once per
# backend (flip the type: line).
#   type: ldap                  # DEFAULT | LDAP2: org.opensearch.security.auth.ldap2.LDAPAuthenticationBackend2
#   config:
#     enable_ssl: true
#     enable_ssl_client_auth: true          # mTLS - client cert required
#     verify_hostnames: true
#     hosts: [localhost:8636]
#     pemtrustedcas_filepath: root-ca.pem
#     pemcert_filepath: kirk.pem
#     pemkey_filepath: kirk-key.pem
#     bind_dn: "cn=opensearch-bind,ou=people,o=TEST"
#     password: "bindpassword"
#     userbase: "ou=people,o=TEST"
#     usersearch: '(uid={0})'
#     username_attribute: uid

# === Apply / authenticate (verify loop - re-run after each config.yml edit) ==
# apply: push the authc.ldap block (-t config, live; TLS-material/host edits need a node restart).
sh ./plugins/opensearch-security/tools/securityadmin.sh \
  -f ./config/opensearch-security/config.yml \
  -t config \
  -icl \
  -nhnv \
  -cacert config/root-ca.pem \
  -cert config/kirk.pem \
  -key config/kirk-key.pem \
  -h localhost \
  -p 9200

# authenticate: 200 + user_name=testuser (backend_roles empty - no authz) = LDAPS + mTLS bind OK.
curl -sk -u "$LDAP_USER:$LDAP_PASS" https://localhost:9200/_plugins/_security/authinfo?pretty

# What to look for (baseline 200):
#   Configuring SNI for hostname: localhost ...            # SNI server_name set (the fix)
#   checkServerTrusted ... succeeded                       # esnode chains to root-ca
#   verifyDNS found hostname match: localhost              # hostname layer 1 pass
#   Opened a connection, total count is now 1              # DEFAULT | LDAP2: Authenticated username testuser

Test matrix. Run every scenario in all four cells - flip the backend on type:; for FIPS set OPENSEARCH_FIPS_MODE=true (launcher loads fips_java.security -> BCJSSE), for non-FIPS set OPENSEARCH_JAVA_OPTS="-Djava.security.properties=$OPENSEARCH_HOME/config/java.security" (BCFIPS stays declared - the FIPS installer converts the node stores to BCFKS - but TLS runs on SunJSSE). Launch-time provider swap, no code path (see Keystores / TLS). Outcomes are identical; only the protocol floor ([TLSv1.3, TLSv1.2] FIPS vs + TLSv1.1 non-FIPS) and the provider differ, so the Prov*/TlsFatalAlert class names in the excerpts are BCJSSE-only.

DEFAULT ldap LDAP2 (...ldap2.LDAPAuthenticationBackend2)
FIPS yes yes
non-FIPS yes yes

Scenario 1 - hostname verification. Same trusted esnode cert throughout; 1b-1d dial a name not in its SAN (echo "127.0.0.1 ldap-wrong.example.com" | sudo tee -a /etc/hosts, then set hosts: [ldap-wrong.example.com:8636]), so the only thing that can object is one of the two hostname guards: (1) ldaptive's verifier (verify_hostnames), (2) JNDI endpoint-id (-Dcom.sun.jndi.ldap.object.disableEndpointIdentification=true in config/jvm.options; the plugin no longer sets it, only warns). Chain trust is valid throughout, so this isolates hostname checking - the untrusted-cert case is 2c. Apply + restart per row.

verify_hostnames JNDI endpoint-id Result / what it proves
1a true on correct name -> 200 (baseline; SNI fix works)
1b true on rejected, layer 1 - ldaptive DefaultHostnameVerifier
1c false on rejected, layer 2 - JNDI/BC endpoint-id (verify_hostnames: false alone isn't enough)
1d false off accepted (200) - hostname unenforced only when both guards are off
1b  HostnameVerifyingTrustManager ... hostnames=[ldap-wrong.example.com] failed         # layer 1 (ldaptive)
    CertificateException: Hostname '[ldap-wrong.example.com]' does not match 'CN=node-0.example.com...'
    (for ldap2 the reject fires inside SniAwareConnection.open() = no fail-open)
1c  AllowAnyHostnameVerifier ... succeeded                                              # layer 1 off
    CertificateException: No subject alternative name found matching domain name ldap-wrong.example.com  # layer 2
        at org.bouncycastle.jsse.provider.ProvX509TrustManager.checkEndpointID(...)
1d  (no hostname reject) -> Opened a connection / Authenticated username testuser

Cleanup: remove the /etc/hosts line + the jvm.options flag, restore hosts: + verify_hostnames: true. Never set disableEndpointIdentification=true in production (it's process-wide).

Scenario 2 - mTLS client authentication. Vary only the client cert / trust anchor; apply + restart per row, and restore pemtrustedcas_filepath: root-ca.pem after 2c. First generate the two "bad" credentials once - both self-signed, so neither chains to the demo root-ca - into config/:

cd $OPENSEARCH_HOME/config
# 2b: untrusted client cert + key (self-signed; does NOT chain to root-ca)
openssl req -x509 -newkey rsa:2048 -nodes -days 30 -subj "/CN=untrusted-client" \
  -keyout untrusted-client.key -out untrusted-client.pem
# 2c: untrusted CA - a trust anchor that did NOT sign esnode
openssl req -x509 -newkey rsa:2048 -nodes -days 30 -subj "/CN=untrusted-ca" \
  -keyout untrusted-ca.key -out untrusted-ca.pem
change (in config.yml) Result / what it proves
2a pemcert/pemkey_filepath: kirk.* (baseline) accepted - PEM client key loads + signs the handshake under BC
2b pemcert/pemkey_filepath: untrusted-client.* rejected - BC withholds the client cert -> server aborts mandatory mTLS
2c pemtrustedcas_filepath: untrusted-ca.pem fails - server cert no longer chains to the trust anchor
2b  checkServerTrusted ... succeeded                        # server side fine
    received fatal(2) certificate_required(116) alert       # client cert withheld
    must NOT log: ClassNotFoundException ...SNISettingTLSSocketFactory  (ldap2 reconnect bug, fixed here)
2c  checkServerTrusted ... failed
    CertPathBuilderException: No issuer certificate for certificate in certification path found.
    TlsFatalAlert: certificate_unknown(46)  ->  Authentication finally failed
Test OnBehalfOf (OBO) token

Exercises OBO token issuance and verification against an already-running cluster, in three modes: keys inline in the dynamic config (Scenario A), held in a BCFKS keystore out of cluster state (Scenario B), or in a PKCS#12 keystore for non-FIPS builds (Scenario C). No restart needed - -t config pushes only the dynamic on_behalf_of block, picked up live. Pick ONE scenario, edit config.yml, then run the apply / issue / use steps. All paths are relative to $OPENSEARCH_HOME.

cd $OPENSEARCH_HOME
export ADMIN_AUTH="admin:<admin-password>"

# === Apply / issue / use (the verify loop - run after editing config.yml) ====
# apply: push the dynamic config (-t config = on_behalf_of block only, live reload).
sh ./plugins/opensearch-security/tools/securityadmin.sh \
  -f ./config/opensearch-security/config.yml \
  -t config \
  -icl \
  -nhnv \
  -cacert config/root-ca.pem \
  -cert config/kirk.pem \
  -key config/kirk-key.pem \
  -h localhost \
  -p 9200

# issue: generate a token.
export OBO_TOKEN=$(curl -sk \
  -u "$ADMIN_AUTH" \
  -X POST \
  -H 'Content-Type: application/json' \
  https://localhost:9200/_plugins/_security/api/generateonbehalfoftoken \
  -d '{
        "description": "obo test",
        "service": "test-service",
        "durationSeconds": "300"
      }' | jq -r '.authenticationToken')
echo "$OBO_TOKEN"

# use: a populated user_name / roles proves the verify side loaded the key,
#      checked the signature, and decrypted the roles.
curl -sk \
  -H "Authorization: Bearer $OBO_TOKEN" \
  https://localhost:9200/_plugins/_security/authinfo?pretty

# === Scenario A - inline keys ================================================
# signing_key >= 512 bits (64 bytes) for HS512; encryption_key >= 256 bits
# (32 bytes) for the FIPS IKM floor.
export OBO_SIGNING_KEY=$(openssl rand 64 | base64 -w0)
export OBO_ENCRYPTION_KEY=$(openssl rand 32 | base64 -w0)

# Put under config.dynamic.on_behalf_of in config/opensearch-security/config.yml:
#   on_behalf_of:
#     enabled: true
#     signing_key: "<value of $OBO_SIGNING_KEY>"
#     encryption_key: "<value of $OBO_ENCRYPTION_KEY>"
#
# Negative checks: a too-short encryption_key (e.g. ZW5jcnlwdGlvbktleQ==, 13 bytes)
# is declined in FIPS mode ("encryption_key is not strong enough for FIPS mode");
# tampering with the token's last segment makes the `use` step return no credentials.

# === Scenario B - BCFKS keystore =========
keytool \
  -genseckey \
  -alias obo-signing \
  -keyalg HmacSHA512 \
  -keysize 512 \
  -storetype BCFKS \
  -providername BCFIPS \
  -keystore config/obo.bcfks \
  -storepass kspass \
  -keypass keypass \
  -providerClass org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider \
  -providerPath lib/bc-fips-2.1.2.jar

keytool \
  -genseckey \
  -alias obo-enc \
  -keyalg AES \
  -keysize 256 \
  -storetype BCFKS \
  -providername BCFIPS \
  -keystore config/obo.bcfks \
  -storepass kspass \
  -keypass keypass \
  -providerClass org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider \
  -providerPath lib/bc-fips-2.1.2.jar

# Reference the keystore in config.yml (no inline keys). <key>_keystore_path is
# config-dir-relative.
#   on_behalf_of:
#     enabled: true
#     signing_key_keystore_path: "obo.bcfks"
#     signing_key_keystore_type: "BCFKS"
#     signing_key_keystore_alias: "obo-signing"
#     signing_key_keystore_password: "kspass"
#     signing_key_keystore_key_password: "keypass"
#     encryption_key_keystore_path: "obo.bcfks"
#     encryption_key_keystore_type: "BCFKS"
#     encryption_key_keystore_alias: "obo-enc"
#     encryption_key_keystore_password: "kspass"
#     encryption_key_keystore_key_password: "keypass"
#
# Same result as Scenario A, but no key material in the config index. GET
# /_plugins/_security/api/securityconfig shows only keystore references.

# === Scenario C - PKCS#12 keystore (NON-FIPS builds only) ====================
# A FIPS build rejects PKCS#12 (no FIPS-approved PKCS#12 in BC FIPS). PKCS#12 has
# no per-entry passwords, so use one value for -storepass and -keypass.
keytool \
  -genseckey \
  -alias obo-signing \
  -keyalg HmacSHA512 \
  -keysize 512 \
  -storetype PKCS12 \
  -keystore config/obo.p12 \
  -storepass kspass \
  -keypass kspass

keytool \
  -genseckey \
  -alias obo-enc \
  -keyalg AES \
  -keysize 256 \
  -storetype PKCS12 \
  -keystore config/obo.p12 \
  -storepass kspass \
  -keypass kspass

# Reference it in config.yml as in Scenario B with the PKCS#12 path/type (the
# loader falls back to the keystore password when _keystore_key_password is absent).
#   on_behalf_of:
#     enabled: true
#     signing_key_keystore_path: "obo.p12"
#     signing_key_keystore_type: "PKCS12"
#     signing_key_keystore_alias: "obo-signing"
#     signing_key_keystore_password: "kspass"
#     encryption_key_keystore_path: "obo.p12"
#     encryption_key_keystore_type: "PKCS12"
#     encryption_key_keystore_alias: "obo-enc"
#     encryption_key_keystore_password: "kspass"

Issues Resolved

Resolves RFC

Related to the series of FIPS PRs in the security plugin:

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • Commits are signed per the DCO using --signoff

References

  1. FIPS 140-3 - Security Requirements for Cryptographic Modules
  2. NIST SP 800-38A - Block Cipher Modes of Operation (ECB/CBC/CFB/OFB/CTR)
  3. NIST SP 800-38F - Methods for Key Wrapping
  4. NIST SP 800-90A Rev. 1 - Random Number Generation Using DRBGs
  5. NIST SP 800-133 Rev. 2 - Recommendation for Cryptographic Key Generation
  6. NIST SP 800-57 Part 1 Rev. 5 - Recommendation for Key Management

@cwperks cwperks added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label May 28, 2026
@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 3ac26d1)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: PemKeyReader rewrite on BouncyCastle and BCFKS/PKCS11 keystore support

Relevant files:

  • src/main/java/org/opensearch/security/support/PemKeyReader.java
  • src/test/java/org/opensearch/security/support/PemKeyReaderLoadSecretKeyTest.java
  • src/test/java/org/opensearch/security/support/PemKeyReaderLoadKeyStoreTest.java
  • src/test/java/org/opensearch/security/ssl/SSLTest.java

Sub-PR theme: LDAP SNI support for BCTLS

Relevant files:

  • src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java
  • src/main/java/org/opensearch/security/auth/ldap2/SNISettingTLSSocketFactory.java
  • src/main/java/org/opensearch/security/auth/ldap2/SniAwareConnection.java
  • src/main/java/org/opensearch/security/auth/ldap2/SocketFactoryClassLoader.java
  • src/main/java/org/opensearch/security/auth/ldap2/HostnameAwareConnectionFactory.java
  • src/test/java/org/opensearch/security/auth/ldap2/SniAwareConnectionTest.java
  • src/test/java/org/opensearch/security/auth/ldap2/SNISettingTLSSocketFactoryTest.java
  • src/test/java/org/opensearch/security/auth/ldap/LdapBackendTest.java

Sub-PR theme: Test updates for FIPS-compatible keys, passwords, and JJWT API migration

Relevant files:

  • src/test/java/org/opensearch/security/http/OnBehalfOfAuthenticatorTest.java
  • src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java
  • src/test/java/org/opensearch/security/tools/HasherTests.java
  • src/integrationTest/java/org/opensearch/security/api/InternalUsersRestApiIntegrationTest.java

⚡ Recommended focus areas for review

Possible NPE

In loadKeyStore, when storePath is null and type equals PKCS11, the code proceeds to load a PKCS#11 keystore. However, if type is null and storePath is null, the current guard storePath == null && !PKCS11.equalsIgnoreCase(type) returns null, but if storePath is non-null and type is null, extractStoreType is called and may work. The concern is that for PKCS11, storePath is passed to error messages but is null, which yields confusing diagnostics. More importantly, loadSecretKeyFromKeystore will call loadKeyStore(null, ..., "PKCS11", ...) returning a non-null store, but the subsequent NPE-check on store == null will not fire even if PKCS11 initialization silently produced an empty store — leading to misleading error messages if the alias is missing.

public static KeyStore loadKeyStore(final String storePath, final String keyStorePassword, final String type) throws Exception {
    if (storePath == null && !PKCS11.equalsIgnoreCase(type)) {
        return null;
    }
    String storeType = extractStoreType(storePath, type);
    final char[] password = keyStorePassword == null ? null : keyStorePassword.toCharArray();
    final KeyStore store;
    if (PKCS11.equalsIgnoreCase(storeType)) {
        try {
            store = KeyStore.getInstance(storeType);
            store.load(null, password);
        } catch (Exception e) {
            throw new OpenSearchException(
                "Failed to initialize PKCS#11 keystore. Ensure a PKCS#11 provider is registered and configured "
                    + "(e.g. SunPKCS11, IBMPKCS11Impl, or your HSM vendor's provider).",
                e
            );
        }
    } else {
        store = KeyStore.getInstance(storeType);
        try (final var in = new FileInputStream(storePath)) {
            store.load(in, password);
        }
    }
    return store;
}
SNI hostname source

In getConnection0, SNI is configured with split[0] as the hostname. If the configured LDAP host is an IP address (not a DNS name), SNI will be set to a raw IP, which per RFC 6066 must not be used for SNI and some servers/BCTLS stacks reject it. Consider skipping SNI when the host is an IP literal to avoid handshake failures when hosts are specified by IP.

}

DefaultConnectionFactory connFactory = new DefaultConnectionFactory(config);

// Register custom socket factory for SNI hostname verification with BouncyCastle FIPS
// This addresses a known issue where JNDI LDAP doesn't pass hostname to SSLSocketFactory
// See: https://github.com/bcgit/bc-java/issues/460
if (enableSSL) {
    configureSNISocketFactory(connFactory);
    try (var ignored = SNISettingTLSSocketFactory.configure(split[0])) {
        connection = connFactory.getConnection();
        connection.open();
    }
} else {
    connection = connFactory.getConnection();
    connection.open();
}
Password fallback semantics

loadSecretKeyFromKeystore falls back to keyStorePassword when keyPassword is null. This matches the keytool default but silently accepts a distinct-key-password intent when the caller passes null explicitly. If a keystore was created with a distinct key password and the caller intentionally passes null keyPassword (expecting failure), the code will now attempt the store password instead, potentially unlocking keys the caller did not intend to. Document this behavior explicitly or require an explicit sentinel.

public static SecretKey loadSecretKeyFromKeystore(
    final String storePath,
    final String keyStorePassword,
    final String type,
    final String alias,
    final String keyPassword
) {
    final KeyStore store;
    try {
        store = loadKeyStore(storePath, keyStorePassword, type);
    } catch (Exception e) {
        throw new IllegalArgumentException(
            "Failed to load secret key from keystore-type '%s' at path '%s'".formatted(type, storePath),
            e
        );
    }
    if (store == null) {
        throw new IllegalArgumentException("Failed to load secret key from keystore-type '%s' at path '%s'".formatted(type, storePath));
    }
    try {
        final char[] kp = keyPassword != null
            ? keyPassword.toCharArray()
            : (keyStorePassword != null ? keyStorePassword.toCharArray() : null);
        final Key key = store.getKey(alias, kp);
        if (key == null) {
            throw new IllegalArgumentException("No key found at alias '" + alias + "' in keystore");
        }
        if (!(key instanceof SecretKey secretKey)) {
            throw new IllegalArgumentException(
                "Entry at alias '" + alias + "' is not a SecretKey (found " + key.getClass().getName() + ")"
            );
        }
        return secretKey;
    } catch (GeneralSecurityException e) {
        throw new IllegalArgumentException(
            "Failed to load secret key from keystore-type '%s' at path '%s'".formatted(type, storePath),
            e
        );
    }
}

@github-actions

github-actions Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 3ac26d1

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Handle multi-URL ldaptive URLs for SNI

The LDAP URL may contain multiple space-separated URLs (ldaptive supports failover
URLs of the form "ldaps://a ldaps://b"). Parsing the whole string as a URI will fail
or take only the first URL's host; the SNI hostname will thus be wrong or missing
when connecting to secondary URLs. Split the URL on whitespace and extract host only
for the first, or accept that SNI may be incorrect across failover and document it.

src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [211-223]

 String sniHostname = null;
 if (config.getLdapUrl() != null && config.getLdapUrl().startsWith("ldaps:")) {
     configureSNISocketFactory(connFactory);
     String ldapUrl = config.getLdapUrl();
+    String firstUrl = ldapUrl.split("\\s+")[0];
     try {
-        sniHostname = new URI(ldapUrl).getHost();
+        sniHostname = new URI(firstUrl).getHost();
         if (StringUtils.isBlank(sniHostname)) {
             log.warn("Could not extract hostname from LDAP URL '{}'; proceeding without SNI configuration", ldapUrl);
         }
     } catch (URISyntaxException e) {
         log.warn("Malformed LDAP URL '{}'; proceeding without SNI configuration: {}", ldapUrl, e.getMessage());
     }
 }
Suggestion importance[1-10]: 6

__

Why: Valid point about ldaptive's failover URL support; parsing a space-separated URL string with new URI() would fail. The suggested split addresses the first URL correctly, though SNI for failover URLs remains a limitation.

Low
Avoid swallowing Errors in password check

Catching Throwable (including Error subclasses like FipsUnapprovedOperationError and
OutOfMemoryError) and silently returning false hides serious FIPS-mode violations
and JVM errors from operators/logs. At minimum, Error should be rethrown, or the
caught type narrowed to Exception, and the failure logged so misconfigurations are
diagnosable.

src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java [49-53]

 CharBuffer passwordBuffer = CharBuffer.wrap(password);
 try {
     return Password.check(passwordBuffer, hash).with(getPBKDF2FunctionFromHash(hash));
-} catch (Throwable e) {
+} catch (Exception e) {
+    log.debug("PBKDF2 password check failed", e);
     return false;
 } finally {
     cleanup(passwordBuffer);
 }
Suggestion importance[1-10]: 6

__

Why: Valid concern - catching Throwable including Error subclasses can hide FIPS violations and JVM errors. However, the improved code introduces a log reference that may not exist in the class, and the change might have been intentional to gracefully handle FIPS errors during password check.

Low
Defensively check for empty provider array

Security.getProviders(filter) returns null only when no providers match; but the API
returns null for an empty result. Verify semantics: for a filter like
"KeyStore.PKCS11", getProviders returns null when no provider supports it and a
non-null array otherwise. The check is correct, but consider also handling the case
where a length-0 array is returned (defensive), and use
Security.getProviders("KeyStore.PKCS11") == null ||
Security.getProviders("KeyStore.PKCS11").length == 0.

src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1256-1258]

 boolean pkcs11Keystore = "PKCS11".equalsIgnoreCase(line.getOptionValue("kst"));
 if (!line.hasOption("vc") && !line.hasOption("ks") && !line.hasOption("cert") && !pkcs11Keystore) {
     throw new ParseException("Specify at least -ks, -cert, or -kst PKCS11");
 }
 if (pkcs11Keystore && line.hasOption("ks")) {
     throw new ParseException(
         "Do not specify -ks together with -kst PKCS11; PKCS11 keystores are token-based and have no file path"
     );
 }
-if (pkcs11Keystore && Security.getProviders("KeyStore.PKCS11") == null) {
-    throw new ParseException("No PKCS#11 provider is registered in the JVM; cannot use -kst PKCS11");
+if (pkcs11Keystore) {
+    Provider[] pkcs11Providers = Security.getProviders("KeyStore.PKCS11");
+    if (pkcs11Providers == null || pkcs11Providers.length == 0) {
+        throw new ParseException("No PKCS#11 provider is registered in the JVM; cannot use -kst PKCS11");
+    }
 }
Suggestion importance[1-10]: 5

__

Why: Adding a length check is a reasonable defensive measure, though Security.getProviders(filter) typically returns null (not empty array) when no providers match, making this a minor robustness improvement.

Low
Document destructive mutation of input array

deriveKey zeroes the input secretBytes in its finally block, so callers who pass a
byte array they still need will find it wiped. More critically, the String-based
constructor allocates the array internally via decodeBase64, which is fine, but the
public byte[] constructor silently mutates caller-owned data. Either document this
destructive contract prominently or defensively copy the array before derivation to
avoid surprising callers (including your own test testByteArrayConstructorWipesInput
which relies on this behavior).

src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [69-71]

 public EncryptionDecryptionUtil(final String encodedSecret) {
     this(decodeBase64(encodedSecret));
 }
 
+/**
+ * NOTE: the provided {@code secretBytes} array will be zeroed out after key derivation.
+ * Callers must not reuse the array afterwards.
+ */
 public EncryptionDecryptionUtil(final byte[] secretBytes) {
     this.aesKey = deriveKey(secretBytes);
 }
Suggestion importance[1-10]: 5

__

Why: Adding a javadoc note about the destructive Arrays.fill on the caller-provided secretBytes is a reasonable documentation improvement, though it's a minor concern as the behavior is already tested and intentional for FIPS security.

Low
Cache parsed LDAP hostname in constructor

Parsing the LDAP URL on every getConnection() call is wasteful and re-throws
IllegalArgumentException for malformed URLs on each connection attempt from a pool.
Parse the URL once in the constructor and cache the hostname so misconfigurations
fail fast at factory creation.

src/main/java/org/opensearch/security/auth/ldap2/HostnameAwareConnectionFactory.java [43-46]

+private final String hostname;
+
+public HostnameAwareConnectionFactory(ConnectionConfig config, String ldapUrl) {
+    super(config);
+    this.ldapUrl = ldapUrl;
+    this.hostname = new LdapURL(ldapUrl).getEntry().getHostname();
+}
+
 @Override
 public Connection getConnection() {
-    String hostname = new LdapURL(ldapUrl).getEntry().getHostname();
     return new SniAwareConnection(super.getConnection(), hostname);
 }
Suggestion importance[1-10]: 5

__

Why: Reasonable optimization that also improves fail-fast behavior for malformed URLs. Impact is minor since LDAP connection creation is not a hot path.

Low
Warn on ignored PKCS11 truststore path

For PKCS11 truststores the truststore path setting is silently ignored, but no
validation confirms the operator's intent. If a user configures a PKCS11 type with a
path value expecting it to be honored, they get no feedback. Consider logging a
warning when truststore is non-empty while type is PKCS11 so the misconfiguration is
visible.

src/main/java/org/opensearch/security/ssl/util/SSLRequestHelper.java [256-262]

 if ("PKCS11".equalsIgnoreCase(truststoreType)) {
+    if (truststore != null && !truststore.isEmpty()) {
+        log.warn("Ignoring truststore path '{}' because truststore type is PKCS11", truststore);
+    }
     ts.load(null, truststorePassword);
 } else {
     try (final var fin = new FileInputStream(env.configDir().resolve(truststore).toAbsolutePath().toString())) {
         ts.load(fin, truststorePassword);
     }
 }
Suggestion importance[1-10]: 4

__

Why: Minor usability improvement to help operators diagnose misconfigurations, but not critical since PKCS11 configurations are relatively rare and this is a diagnostic enhancement rather than a functional fix.

Low
Set initialization flag after attempt

initialized is set to true before buildJwtParser runs, so any exception permanently
disables OBO with no way to retry (e.g. after fixing the misconfiguration or
resolving a transient keystore-read issue) without a node restart. Set initialized =
true only after the initialization successfully completes, or in a finally block
after logging, but only if you also intentionally want to prevent retries. If
retries are undesirable, keep as-is but consider setting it after the try to at
least allow success on the first run to be atomic.

src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [79-90]

 private synchronized boolean ensureInitialized() {
     if (!initialized) {
-        initialized = true;
         try {
             jwtParser = AccessController.doPrivileged(this::buildJwtParser);
             encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath);
         } catch (final RuntimeException e) {
             log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e);
+        } finally {
+            initialized = true;
         }
     }
     return jwtParser != null;
 }
Suggestion importance[1-10]: 3

__

Why: The suggested change is functionally equivalent to the original (both set initialized = true unconditionally). The suggestion doesn't actually enable retries as it claims - a finally block still sets it to true on failure.

Low
Avoid sharing password char arrays between stores

resolvePassword returns defaultPassword when no password is configured, and the
callers for keystore and truststore share the same default instance through
defaultStorePassword() invocations. However, since resolvePassword returns the same
reference and downstream code may zero out the char array via password.chars()
cleanup, the second use could see a wiped array. Ensure each call site receives a
fresh StorePassword instance (already done by calling the method twice — verify that
defaultStorePassword() is invoked separately per call, not cached).

src/main/java/org/opensearch/security/ssl/config/SslCertificatesLoader.java [106-108]

+private static StorePassword defaultStorePassword() {
+    return StorePassword.of(DEFAULT_STORE_PASSWORD.toCharArray());
+}
 
-
Suggestion importance[1-10]: 2

__

Why: The suggestion's improved_code is identical to the existing_code, providing no actual change. It only asks to verify existing behavior, which is low value.

Low
Possible issue
Avoid NPE for PKCS11 keystore load

When storePath is null and type is PKCS11, extractStoreType(null, "PKCS11") will be
called and its detectStoreType fallback attempts to open the (null) file, causing a
NullPointerException. Skip the extraction/type-detection path for PKCS#11 since
there is no file to detect from, and just use the given type directly.

src/main/java/org/opensearch/security/support/PemKeyReader.java [155-159]

 public static KeyStore loadKeyStore(final String storePath, final String keyStorePassword, final String type) throws Exception {
     if (storePath == null && !PKCS11.equalsIgnoreCase(type)) {
         return null;
     }
-    String storeType = extractStoreType(storePath, type);
+    String storeType = PKCS11.equalsIgnoreCase(type) ? PKCS11 : extractStoreType(storePath, type);
Suggestion importance[1-10]: 6

__

Why: Valid concern: when storePath is null for PKCS11, calling extractStoreType(null, "PKCS11") could trigger detectStoreType(null) if type were null, but since type is PKCS11 here, extractStoreType returns early. Still, the defensive fix improves clarity and avoids potential NPE risk.

Low
Guard against empty trust manager array

Blindly picking tmf.getTrustManagers()[0] assumes at least one trust manager is
returned and that the first one is an X509TrustManager. If the factory returns an
empty array (possible with a misconfigured/empty truststore) this will throw
ArrayIndexOutOfBoundsException with no useful context. Validate the array is
non-empty and log/throw a clearer error, and consider passing all returned managers
rather than just the first.

src/main/java/org/opensearch/security/auth/ldap2/LDAPConnectionFactoryFactory.java [357-364]

 } else {
     try {
         TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX");
         tmf.init(this.sslConfig.getEffectiveTruststore());
-        ldaptiveSslConfig.setTrustManagers(tmf.getTrustManagers()[0]);
+        final javax.net.ssl.TrustManager[] tms = tmf.getTrustManagers();
+        if (tms == null || tms.length == 0) {
+            throw new IllegalStateException("PKIX TrustManagerFactory returned no trust managers for LDAPS");
+        }
+        ldaptiveSslConfig.setTrustManagers(tms[0]);
     } catch (GeneralSecurityException e) {
         throw new IllegalStateException("Failed to initialize PKIX TrustManager for LDAPS", e);
     }
 }
Suggestion importance[1-10]: 4

__

Why: In practice TrustManagerFactory.getTrustManagers() returns at least one manager after init() succeeds, so this is a defensive check with limited real-world impact, but it does improve error clarity.

Low
Security
Restrict visibility of mutable env supplier

The mutable public static envSupplier field is not thread-safe and is intended only
for tests, but exposes global mutable state to production code where a concurrent
write could flip FIPS detection at runtime. Consider making it volatile and
package-private (or using a dedicated test hook via reflection) to prevent
accidental external mutation.

src/main/java/org/opensearch/security/support/FipsMode.java [17-21]

 public final class FipsMode {
 
-    public static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE");
+    static volatile java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE");
 
     public static boolean isEnabled() {
         return "true".equalsIgnoreCase(envSupplier.get());
     }
Suggestion importance[1-10]: 6

__

Why: Valid point that a publicly mutable static field for FIPS detection is a security concern. Making it package-private and volatile would better encapsulate the test hook while preventing unintended external mutation.

Low
Zeroing is undermined by String returns

Base64.getUrlEncoder().encodeToString(credentialBytes) produces a String that
internally holds a copy of the password bytes (post-encoding), and new
BasicAuthToken("Basic " + authToken) produces yet another. The careful zeroing of
credentialBytes, passwordBytes, and passwordByteBuffer.array() is undermined because
the plaintext password is still recoverable from the returned Base64 String in the
heap. Either document this as inherent to returning the token as a String, or return
the encoded bytes and let callers manage the lifetime.

src/main/java/org/opensearch/security/user/UserService.java [328-340]

-byte[] accountBytes = accountName.getBytes(StandardCharsets.UTF_8);
-ByteBuffer passwordByteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(plainTextPassword));
-byte[] passwordBytes = new byte[passwordByteBuffer.remaining()];
-passwordByteBuffer.get(passwordBytes);
-Arrays.fill(passwordByteBuffer.array(), (byte) 0);
-credentialBytes = new byte[accountBytes.length + 1 + passwordBytes.length];
-System.arraycopy(accountBytes, 0, credentialBytes, 0, accountBytes.length);
-credentialBytes[accountBytes.length] = ':';
-System.arraycopy(passwordBytes, 0, credentialBytes, accountBytes.length + 1, passwordBytes.length);
-Arrays.fill(passwordBytes, (byte) 0);
-
+// NOTE: the returned BasicAuthToken contains a Base64-encoded copy of the password;
+// the plaintext remains recoverable from the returned String until garbage-collected.
 authToken = Base64.getUrlEncoder().encodeToString(credentialBytes);
 return new BasicAuthToken("Basic " + authToken);
Suggestion importance[1-10]: 4

__

Why: Valid observation that the Base64 String still holds the credential in memory, undermining some of the zeroing effort. However, the suggestion only proposes adding a comment rather than a functional fix, limiting its impact.

Low

Previous suggestions

Suggestions up to commit b5760ff
CategorySuggestion                                                                                                                                    Impact
General
Handle multi-URL LDAP configurations for SNI

An LDAP URL may contain multiple space-separated URLs (ldaptive supports this). new
URI(ldapUrl) will fail or return the wrong host for multi-URL configurations,
silently disabling SNI. Parse only the first URL token to reliably extract the
hostname.

src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [212-223]

 if (config.getLdapUrl() != null && config.getLdapUrl().startsWith("ldaps:")) {
     configureSNISocketFactory(connFactory);
     String ldapUrl = config.getLdapUrl();
+    String firstUrl = ldapUrl.split("\\s+")[0];
     try {
-        sniHostname = new URI(ldapUrl).getHost();
+        sniHostname = new URI(firstUrl).getHost();
         if (StringUtils.isBlank(sniHostname)) {
             log.warn("Could not extract hostname from LDAP URL '{}'; proceeding without SNI configuration", ldapUrl);
         }
     } catch (URISyntaxException e) {
         log.warn("Malformed LDAP URL '{}'; proceeding without SNI configuration: {}", ldapUrl, e.getMessage());
     }
 }
Suggestion importance[1-10]: 7

__

Why: Ldaptive genuinely supports space-separated multi-URL configurations, and new URI() would fail on such strings, silently disabling SNI for a legitimate configuration. Meaningful correctness improvement.

Medium
Allow retry after transient init failures

Initialization is attempted exactly once, so any transient failure (e.g. a keystore
file not yet mounted at plugin construction) permanently disables OBO authentication
until the node restarts — even after the underlying issue is fixed. Consider
allowing retry on subsequent invocations while still avoiding repeated log spam
(e.g. log the error once but keep initialized=false on failure so a later call can
retry, or expose a management API to reinitialize).

src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [79-90]

 private synchronized boolean ensureInitialized() {
     if (!initialized) {
-        initialized = true;
         try {
             jwtParser = AccessController.doPrivileged(this::buildJwtParser);
             encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath);
+            initialized = true;
         } catch (final RuntimeException e) {
             log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e);
+            // leave initialized=false so a subsequent request can retry after the config is fixed
         }
     }
     return jwtParser != null;
 }
Suggestion importance[1-10]: 6

__

Why: Allowing retry on transient initialization failures (e.g. delayed keystore availability) improves resilience, avoiding a permanent disablement of OBO authentication until node restart. This is a useful robustness improvement, though log spam on repeated failures should be considered.

Low
Harden PKCS#11 provider availability check

Security.getProviders(filter) returns null only when no providers match, but the doc
contract is to return null in that case; however, callers commonly get an empty
array on some JVMs. More importantly, the same check is duplicated for truststore.
Verify with Security.getProviders("KeyStore.PKCS11") == null ||
Security.getProviders("KeyStore.PKCS11").length == 0 to be robust across JVMs.

src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1256-1258]

-if (pkcs11Keystore && Security.getProviders("KeyStore.PKCS11") == null) {
-    throw new ParseException("No PKCS#11 provider is registered in the JVM; cannot use -kst PKCS11");
+if (pkcs11Keystore) {
+    Provider[] p = Security.getProviders("KeyStore.PKCS11");
+    if (p == null || p.length == 0) {
+        throw new ParseException("No PKCS#11 provider is registered in the JVM; cannot use -kst PKCS11");
+    }
 }
Suggestion importance[1-10]: 5

__

Why: Reasonable hardening—Security.getProviders(filter) could theoretically return an empty array on some JVMs, though the JDK contract specifies null. Minor robustness improvement.

Low
Document that input bytes are zeroed

deriveKey zeroes the input secretBytes in its finally block. When constructed via
the String-based constructor, this correctly wipes the decoded copy. However,
callers of the byte[] constructor (e.g. fromSettings passing
keystoreKey.getEncoded()) may not expect their array to be zeroed as a side effect,
and in the keystore case this mutates the returned copy from SecretKey.getEncoded()
— harmless there, but a surprising contract. Document this mutation on the
constructor or defensively copy before wiping.

src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [69-71]

 public EncryptionDecryptionUtil(final String encodedSecret) {
     this(decodeBase64(encodedSecret));
 }
 
+/**
+ * Note: {@code secretBytes} is zeroed after key derivation; callers must not reuse it.
+ */
 public EncryptionDecryptionUtil(final byte[] secretBytes) {
     this.aesKey = deriveKey(secretBytes);
 }
Suggestion importance[1-10]: 5

__

Why: Documenting the side-effect of zeroing secretBytes improves the constructor's contract clarity, since callers passing externally-owned arrays may be surprised by the mutation. This is a minor documentation improvement.

Low
Reject empty signing key explicitly

The bit-length check here validates the raw decoded length, but
KeyUtils.createJwtParserBuilderFromSigningKey may itself validate/pad. More
importantly, when the signing key comes from a keystore (the other branch),
validateSigningKeyBitLength(keyBytes.length * Byte.SIZE) is called, but the original
settings-based path decodes Base64 first — meaning a keystore key that stores raw
bytes and the Base64-decoded settings key are treated consistently. However, if
signingKey is an empty string, Base64.getDecoder().decode("") returns an empty array
and yields 0 bits, producing a misleading error. Consider handling the empty case
explicitly with the same "not found" message.

src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [116-127]

 static void validateSigningKey(final String signingKey) {
-    if (signingKey == null) {
+    if (signingKey == null || signingKey.isEmpty()) {
         throw new OpenSearchSecurityException("Unable to find on behalf of authenticator signing_key");
     }
     final int signingKeyLengthBits;
     try {
         signingKeyLengthBits = Base64.getDecoder().decode(signingKey).length * Byte.SIZE;
     } catch (final IllegalArgumentException e) {
         throw new OpenSearchSecurityException("Signing key is not a valid Base64-encoded value: " + e.getMessage());
     }
     validateSigningKeyBitLength(signingKeyLengthBits);
 }
Suggestion importance[1-10]: 5

__

Why: Handling an empty signingKey string explicitly produces a clearer error message ("not found") instead of a confusing "0 bits" message. This is a minor usability improvement for misconfiguration diagnostics.

Low
Cache parsed hostname in constructor

new LdapURL(ldapUrl).getEntry() is invoked on every getConnection() call, re-parsing
the URL for each pooled connection acquisition. Parse the LDAP URL once in the
constructor and cache the hostname to avoid unnecessary allocations and parsing on
hot paths.

src/main/java/org/opensearch/security/auth/ldap2/HostnameAwareConnectionFactory.java [43-46]

+private final String hostname;
+
+public HostnameAwareConnectionFactory(ConnectionConfig config, String ldapUrl) {
+    super(config);
+    this.ldapUrl = ldapUrl;
+    this.hostname = new LdapURL(ldapUrl).getEntry().getHostname();
+}
+
 @Override
 public Connection getConnection() {
-    String hostname = new LdapURL(ldapUrl).getEntry().getHostname();
     return new SniAwareConnection(super.getConnection(), hostname);
 }
Suggestion importance[1-10]: 4

__

Why: Caching the hostname avoids re-parsing on each connection acquisition, a minor performance improvement on a non-hot path. Behavior is preserved.

Low
Avoid path-based detection for PKCS11

When storePath is null and type is PKCS11, extractStoreType(null, "PKCS11") is
called; detectStoreType(null) would NPE if type were null. Since type is guaranteed
non-null here, this is safe, but for PKCS11 the storePath is unused - pass type
directly to avoid path-based detection logic when possible.

src/main/java/org/opensearch/security/support/PemKeyReader.java [155-159]

 public static KeyStore loadKeyStore(final String storePath, final String keyStorePassword, final String type) throws Exception {
     if (storePath == null && !PKCS11.equalsIgnoreCase(type)) {
         return null;
     }
-    String storeType = extractStoreType(storePath, type);
+    String storeType = PKCS11.equalsIgnoreCase(type) ? PKCS11 : extractStoreType(storePath, type);
Suggestion importance[1-10]: 3

__

Why: Minor optimization; the current code already handles PKCS11 correctly via the non-null type argument. The suggestion offers marginal clarity improvement but is not a functional fix.

Low
Validate truststore path vs type

When truststoreType is PKCS11 but the configuration provides a truststore path, that
path is silently ignored. Consider validating that a file path is not provided
together with PKCS11 (or that a file path is provided when non-PKCS11), and fail
fast with a clear error message to avoid confusing misconfiguration.

src/main/java/org/opensearch/security/ssl/util/SSLRequestHelper.java [251-262]

 final char[] truststorePassword = Optional.ofNullable(SECURITY_SSL_HTTP_TRUSTSTORE_PASSWORD.getSetting(settings))
     .filter(p -> !p.isEmpty())
     .map(String::toCharArray)
     .orElse(null);
 final KeyStore ts = KeyStore.getInstance(truststoreType);
 if ("PKCS11".equalsIgnoreCase(truststoreType)) {
     ts.load(null, truststorePassword);
 } else {
+    if (truststore == null) {
+        throw new IllegalArgumentException("Truststore path required for type " + truststoreType);
+    }
     try (final var fin = new FileInputStream(env.configDir().resolve(truststore).toAbsolutePath().toString())) {
         ts.load(fin, truststorePassword);
     }
 }
Suggestion importance[1-10]: 3

__

Why: The validation adds defensive checking, but the truststore parameter is typically validated by callers; the improvement is minor and mainly cosmetic.

Low
Possible issue
Guard against null resolved store type

detectStoreType may return null when storePath is null (as is legal for PKCS11),
causing a NullPointerException on finalStoreType.toUpperCase(...) or
equalsIgnoreCase. Guard against a null resolved type before use, or short-circuit
for PKCS11 when no path is provided.

src/main/java/org/opensearch/security/support/PemKeyReader.java [381-390]

 public static String extractStoreType(String storePath, String storeType) {
     var finalStoreType = Optional.ofNullable(storeType).orElseGet(() -> detectStoreType(storePath));
+    if (finalStoreType == null) {
+        throw new IllegalArgumentException("Unable to determine keystore/truststore type for path: " + storePath);
+    }
     var isFipsSupportedStoreType = Stream.of(PKCS11, BCFKS).anyMatch(it -> it.equalsIgnoreCase(finalStoreType));
 
     if (FipsMode.isEnabled() && !isFipsSupportedStoreType) {
         throw new IllegalArgumentException(
             finalStoreType.toUpperCase(Locale.ROOT) + " keystores / truststores are not supported in FIPS mode - use BCFKS or PKCS#11"
         );
     }
     return finalStoreType;
 }
Suggestion importance[1-10]: 6

__

Why: Valid defensive check: detectStoreType can return null for unknown magic bytes, which would NPE on finalStoreType.toUpperCase(...). A moderate improvement for robustness.

Low
Avoid swallowing Errors in password check

Catching Throwable and silently returning false will swallow critical errors like
OutOfMemoryError, StackOverflowError, and FipsUnapprovedOperationError, making
failures indistinguishable from wrong passwords and hiding configuration/environment
issues. At minimum, log the exception and rethrow Error subclasses, or narrow the
catch to expected exceptions from Password.check.

src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java [47-53]

 CharBuffer passwordBuffer = CharBuffer.wrap(password);
 try {
     return Password.check(passwordBuffer, hash).with(getPBKDF2FunctionFromHash(hash));
-} catch (Throwable e) {
+} catch (Exception e) {
     return false;
 } finally {
     cleanup(passwordBuffer);
 }
Suggestion importance[1-10]: 6

__

Why: Catching Throwable and returning false can mask serious JVM errors like OutOfMemoryError; narrowing to Exception is a valid defensive improvement, though the test shouldThrowExceptionForWeekPassword expects FipsUnapprovedOperationError to propagate — which is an Error, so narrowing is actually necessary for correctness.

Low
Security
Restrict visibility of FIPS mode supplier

The envSupplier field is public and mutable, allowing any code to change FIPS mode
detection at runtime. This is a security-sensitive setting that could be tampered
with. Restrict visibility to package-private (for tests) and mark it as volatile, or
use a testing-only setter with proper access controls.

src/main/java/org/opensearch/security/support/FipsMode.java [17-21]

-public static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE");
+static volatile java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE");
 
 public static boolean isEnabled() {
     return "true".equalsIgnoreCase(envSupplier.get());
 }
Suggestion importance[1-10]: 5

__

Why: Making envSupplier package-private and volatile is a reasonable improvement for encapsulation, but the field is used by FipsModeTest in the same package so the change is feasible. Impact is moderate as this is test-facing code.

Low
Plaintext leaks via immutable String

Base64.getUrlEncoder().encodeToString(credentialBytes) internally produces a String
(authToken) holding the credential in an immutable, non-wipeable buffer, defeating
the careful zeroing done above. Additionally, the returned BasicAuthToken("Basic " +
authToken) retains the plaintext credential in the JVM heap indefinitely. If the
intent is to minimize plaintext exposure, at minimum keep authToken local and avoid
keeping additional String copies; ideally the token API should accept a char[].

src/main/java/org/opensearch/security/user/UserService.java [328-339]

 byte[] accountBytes = accountName.getBytes(StandardCharsets.UTF_8);
 ByteBuffer passwordByteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(plainTextPassword));
 byte[] passwordBytes = new byte[passwordByteBuffer.remaining()];
 passwordByteBuffer.get(passwordBytes);
 Arrays.fill(passwordByteBuffer.array(), (byte) 0);
 credentialBytes = new byte[accountBytes.length + 1 + passwordBytes.length];
 System.arraycopy(accountBytes, 0, credentialBytes, 0, accountBytes.length);
 credentialBytes[accountBytes.length] = ':';
 System.arraycopy(passwordBytes, 0, credentialBytes, accountBytes.length + 1, passwordBytes.length);
 Arrays.fill(passwordBytes, (byte) 0);
 
+// NOTE: Base64 encoding materializes plaintext credentials into an immutable String;
+// subsequent wiping of credentialBytes cannot reach that copy.
 authToken = Base64.getUrlEncoder().encodeToString(credentialBytes);
Suggestion importance[1-10]: 4

__

Why: The observation is correct that Base64-encoding into a String defeats the byte-array wiping, but the suggestion only adds a comment and does not actually fix the issue. Since the auth token API constrains the return type, the fix is limited in scope.

Low
Suggestions up to commit fd9ed30
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid wiping caller-owned key material

Zeroing secretBytes in the finally block wipes the caller's byte array, which is
problematic when the same bytes came from SecretKey.getEncoded() (via fromSettings)
or a caller-owned buffer. This can invalidate the keystore-derived key for
subsequent uses and cause hard-to-diagnose failures. Copy the input first and only
wipe the local copy.

src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [111-134]

-        byte[] derivedKey = new byte[AES_KEY_LENGTH_BYTES];  // AES-256
-        kdf.generateBytes(derivedKey);
-        return new SecretKeySpec(derivedKey, "AES");
-    } catch (final Exception e) {
-        throw new RuntimeException("Error deriving key from secret", e);
-    } finally {
-        Arrays.fill(secretBytes, (byte) 0);
-    }
+        byte[] ikm = secretBytes.clone();
+        try {
+            FipsKDF.AgreementKDFParameters hkdfParams = FipsKDF.HKDF.withPRF(FipsKDF.AgreementKDFPRF.SHA256_HMAC)
+                .using(ikm)
+                .withIV(HKDF_INFO);
+            KDFCalculator<FipsKDF.AgreementKDFParameters> kdf = new FipsKDF.AgreementOperatorFactory().createKDFCalculator(hkdfParams);
+            byte[] derivedKey = new byte[AES_KEY_LENGTH_BYTES];
+            kdf.generateBytes(derivedKey);
+            return new SecretKeySpec(derivedKey, "AES");
+        } catch (final Exception e) {
+            throw new RuntimeException("Error deriving key from secret", e);
+        } finally {
+            Arrays.fill(ikm, (byte) 0);
+        }
Suggestion importance[1-10]: 7

__

Why: Valid concern: the finally block zeroes secretBytes which may be caller-owned (e.g., from SecretKey.getEncoded()), potentially corrupting subsequent uses. Cloning first is a defensive improvement, though the current constructor is only called with freshly-decoded bytes.

Medium
Guard against NPE on missing filepath

When isPkcs11 is false and KEYSTORE_FILEPATH is not set, resolvePath(...) will be
called with null, then path.toString() will NPE. Guard against a missing filepath
explicitly and produce a descriptive error (same for the truststore variant below).

src/main/java/org/opensearch/security/ssl/config/SslCertificatesLoader.java [133-136]

 final String explicitType = environment.settings().get(sslConfigSuffix + KEYSTORE_TYPE);
 final boolean isPkcs11 = PemKeyReader.PKCS11.equalsIgnoreCase(explicitType);
-final Path path = isPkcs11 ? null : resolvePath(environment.settings().get(sslConfigSuffix + KEYSTORE_FILEPATH), environment);
+final String filepath = environment.settings().get(sslConfigSuffix + KEYSTORE_FILEPATH);
+if (!isPkcs11 && filepath == null) {
+    throw new OpenSearchException("Missing keystore filepath setting: " + sslConfigSuffix + KEYSTORE_FILEPATH);
+}
+final Path path = isPkcs11 ? null : resolvePath(filepath, environment);
 final String resolvedType = isPkcs11 ? PemKeyReader.PKCS11 : PemKeyReader.extractStoreType(path.toString(), explicitType);
Suggestion importance[1-10]: 7

__

Why: Valid concern: if isPkcs11 is false and KEYSTORE_FILEPATH is unset, path.toString() would throw NPE with an unclear error. An explicit check with a descriptive message improves robustness and diagnostics.

Medium
Handle multi-URL LDAP configuration safely

config.getLdapUrl() can return a space-separated list of URLs (ldaptive supports
multi-URL configs). Passing the raw list to new URI(...) will throw
URISyntaxException, and even when it doesn't, the extracted host may be wrong. Split
on whitespace and use the first entry, matching the pattern used in getConnection0.

src/main/java/org/opensearch/security/auth/ldap/backend/LDAPAuthorizationBackend.java [211-222]

             String sniHostname = null;
             if (config.getLdapUrl() != null && config.getLdapUrl().startsWith("ldaps:")) {
                 configureSNISocketFactory(connFactory);
                 String ldapUrl = config.getLdapUrl();
+                String firstUrl = ldapUrl.split("\\s+")[0];
                 try {
-                    sniHostname = new URI(ldapUrl).getHost();
+                    sniHostname = new URI(firstUrl).getHost();
Suggestion importance[1-10]: 6

__

Why: Reasonable point that ldaptive supports space-separated LDAP URL lists which would fail URI parsing. Since getConnection0 uses a similar split pattern, applying the same here improves robustness, though this path is checkConnection0 and behavior in multi-URL configs is not verified.

Low
Avoid catching Throwable in password check

Catching Throwable here swallows critical errors like OutOfMemoryError,
StackOverflowError, and ThreadDeath, which should never be silently converted to a
false authentication result. Restrict the catch to Exception (or a more targeted
FipsUnapprovedOperationError/RuntimeException), and consider logging at debug so
failures are diagnosable.

src/main/java/org/opensearch/security/hasher/PBKDF2PasswordHasher.java [47-53]

 try {
     return Password.check(passwordBuffer, hash).with(getPBKDF2FunctionFromHash(hash));
-} catch (Throwable e) {
+} catch (Exception e) {
     return false;
 } finally {
     cleanup(passwordBuffer);
 }
Suggestion importance[1-10]: 6

__

Why: Catching Throwable can swallow serious JVM errors like OutOfMemoryError. However, the FIPS FipsUnapprovedOperationError is an Error subclass, which is why Throwable was likely chosen; narrowing may need care. Moderate impact.

Low
Fix initialization race in lazy init

initialized and jwtParser/encryptionUtil are read outside the synchronized block via
ensureInitialized()'s return, but the fields are not volatile here (only declared
volatile — good), yet setting initialized = true before attempting initialization
means a concurrent caller could observe initialized == true while jwtParser is still
null due to a race with the try block. Set initialized = true only after
initialization completes (or in a finally) to avoid mis-reporting state, and ensure
repeated failed attempts don't silently swallow errors on every request.

src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java [79-90]

 private synchronized boolean ensureInitialized() {
     if (!initialized) {
-        initialized = true;
         try {
             jwtParser = AccessController.doPrivileged(this::buildJwtParser);
             encryptionUtil = EncryptionDecryptionUtil.fromSettings(settings, ENCRYPTION_KEY, configPath);
         } catch (final RuntimeException e) {
             log.error("On-behalf-of authentication is misconfigured; OBO tokens will be rejected: {}", e.toString(), e);
+        } finally {
+            initialized = true;
         }
     }
     return jwtParser != null;
 }
Suggestion importance[1-10]: 5

__

Why: The method is synchronized, so there is no actual race between concurrent callers. However, moving initialized = true to a finally block is a minor stylistic improvement; the current code intentionally avoids repeated failed initialization attempts on every request.

Low
General
Preserve prior SNI hostname on nested configure

configure() unconditionally overwrites any existing thread-local hostname and its
returned close clears the value entirely. If nested/reentrant configuration ever
occurs (e.g., pooled connection triggers another open on the same thread), the inner
close will erase the outer hostname before the outer socket is created. Save and
restore the prior value instead of clearing.

src/main/java/org/opensearch/security/auth/ldap2/SNISettingTLSSocketFactory.java [81-85]

 public static SniContext configure(String hostname) {
+    final String previous = hostnameThreadLocal.get();
     hostnameThreadLocal.set(hostname);
     log.debug("Configured SNI context: hostname={}", hostname);
-    return SNISettingTLSSocketFactory::clearContext;
+    return () -> {
+        if (previous == null) {
+            hostnameThreadLocal.remove();
+        } else {
+            hostnameThreadLocal.set(previous);
+        }
+    };
 }
Suggestion importance[1-10]: 6

__

Why: Reasonable defensive improvement for potential nested/reentrant configuration scenarios. While the current usage pattern (via HostnameAwareConnectionFactory) may not trigger nested calls, save/restore semantics is more robust for ThreadLocal-based context management.

Low
Use case-sensitive class name comparison

Class name comparison should be case-sensitive. Using equalsIgnoreCase here is
inconsistent with the exact match used for SNISettingTLSSocketFactory above and
could cause incorrect class resolution if a similarly-named class exists. Use equals
to match Java class name semantics.

src/main/java/org/opensearch/security/auth/ldap2/SocketFactoryClassLoader.java [40-42]

-if (ThreadLocalTLSSocketFactory.class.getName().equalsIgnoreCase(name)) {
+if (ThreadLocalTLSSocketFactory.class.getName().equals(name)) {
     return ThreadLocalTLSSocketFactory.class;
 }
Suggestion importance[1-10]: 6

__

Why: Java class name resolution is case-sensitive, so using equalsIgnoreCase here is inconsistent with the exact match used for SNISettingTLSSocketFactory and could theoretically cause incorrect resolution. Reasonable correctness improvement.

Low
Use locale-independent uppercase conversion

type.toUpperCase() uses the default locale and can misbehave under Turkish locale
(e.g., "pkcs11" → "PKCS11" fine, but general defensive coding requires Locale.ROOT).
Also, KeyStore.getInstance is case-sensitive for some providers/types; passing the
caller-supplied type as-is (or normalized to ROOT) is safer.

src/main/java/org/opensearch/security/tools/SecurityAdmin.java [1717-1728]

     private static KeyStore loadStore(String path, String type, char[] password) throws Exception {
-        KeyStore ks = KeyStore.getInstance(type.toUpperCase());
+        KeyStore ks = KeyStore.getInstance(type.toUpperCase(java.util.Locale.ROOT));
 
         if ("PKCS11".equalsIgnoreCase(type)) {
             ks.load(null, password);
         } else {
             try (final var fin = new FileInputStream(Paths.get(path).toFile())) {
                 ks.load(fin, password);
             }
         }
         return ks;
     }
Suggestion importance[1-10]: 5

__

Why: Valid best-practice suggestion to use Locale.ROOT for locale-independent uppercase conversion, avoiding potential Turkish-locale surprises. Low-impact but a legitimate code-quality improvement.

Low
Zero full encoded password buffer safely

passwordByteBuffer.array() is only accessible if the buffer is backed by an
accessible array; more importantly, after get(passwordBytes) the backing array
positions before arrayOffset()+position are past-encoded bytes — you should zero the
full backing array via its offset/limit, and preferably check hasArray(). Also, the
temporary CharBuffer.wrap(plainTextPassword) and any intermediate encoder state may
retain characters; ensure zeroing covers the full backing array length.

src/main/java/org/opensearch/security/user/UserService.java [328-332]

 byte[] accountBytes = accountName.getBytes(StandardCharsets.UTF_8);
 ByteBuffer passwordByteBuffer = StandardCharsets.UTF_8.encode(CharBuffer.wrap(plainTextPassword));
 byte[] passwordBytes = new byte[passwordByteBuffer.remaining()];
 passwordByteBuffer.get(passwordBytes);
-Arrays.fill(passwordByteBuffer.array(), (byte) 0);
+if (passwordByteBuffer.hasArray()) {
+    Arrays.fill(passwordByteBuffer.array(), 0, passwordByteBuffer.array().length, (byte) 0);
+}
Suggestion importance[1-10]: 5

__

Why: The suggestion addresses a valid concern about safely zeroing the backing array using hasArray() check, which is defensive but the current code works for standard encoder outputs. Minor security hardening improvement for credential handling.

Low
Restrict visibility of mutable static supplier

A mutable public static supplier is a global that any caller (or test) can silently
overwrite, and it also breaks the "single source of truth" documented in the class
Javadoc. Make the field private and non-final only via a package-private setter for
tests, or use a @VisibleForTesting guarded mechanism.

src/main/java/org/opensearch/security/support/FipsMode.java [17]

-public static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE");
+static java.util.function.Supplier<String> envSupplier = () -> System.getenv("OPENSEARCH_FIPS_MODE");
 
 public static boolean isEnabled() {
     return "true".equalsIgnoreCase(envSupplier.get());
 }
Suggestion importance[1-10]: 5

__

Why: Making the mutable supplier public allows any caller to override FIPS detection globally, which contradicts the "single source of truth" documentation. Reducing visibility is a reasonable defensive improvement.

Low
Avoid null path detection for PKCS11

When storePath == null and type is PKCS11, extractStoreType(null, "PKCS11") will
invoke detectStoreType(null) if type is somehow null-cased differently, and even in
the current path it depends on type being non-null. Additionally
detectStoreType(null) would NPE if the branch were ever changed. Guard explicitly by
passing PKCS11 as the resolved type when storePath is null to avoid future
regressions.

src/main/java/org/opensearch/security/support/PemKeyReader.java [155-159]

     public static KeyStore loadKeyStore(final String storePath, final String keyStorePassword, final String type) throws Exception {
         if (storePath == null && !PKCS11.equalsIgnoreCase(type)) {
             return null;
         }
-        String storeType = extractStoreType(storePath, type);
+        String storeType = storePath == null ? PKCS11 : extractStoreType(storePath, type);
Suggestion importance[1-10]: 3

__

Why: Minor defensive improvement. In the current code flow, extractStoreType is only called with non-null path or with type=PKCS11 (which short-circuits via Optional.ofNullable(storeType)), so this is a hypothetical hardening rather than a real bug.

Low
Guard against null keystore path

When type is "PKCS11" the code intentionally calls load(null, password) and path may
be null. The error message branch is correct, but the outer catch will also fire
before path is dereferenced elsewhere; ensure the earlier Files.newInputStream(path)
block is only reached when path is non-null (it is, since it's inside the else
branch), which is fine — however, verify callers do not pass a null path with a
non-PKCS11 type, which would NPE inside Files.newInputStream. Add an explicit guard.

src/main/java/org/opensearch/security/ssl/config/KeyStoreUtils.java [217-227]

-} catch (Exception e) {
-    throw new OpenSearchException("Failed to load keystore from " + (path != null ? path : "PKCS#11 token"), e);
+} else {
+    if (path == null) {
+        throw new OpenSearchException("Keystore path is required for type " + type);
+    }
+    try (final var in = Files.newInputStream(path)) {
+        keyStore.load(in, password);
+    } catch (IOException e) {
+        throw new RuntimeException(e);
+    }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion adds a defensive null check, but callers presumably pass a valid path for non-PKCS11 types, and a resulting NPE would be caught by the outer catch anyway. Low impact.

Low
Suggestions up to commit 3ab9228
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid zeroing caller-owned key bytes

deriveKey zeroes out the passed-in secretBytes array in its finally block. When
callers pass a SecretKey.getEncoded() result (as in fromSettings), this wipes the
shared key material array, which can corrupt the keystore's cached key and any
subsequent access. Clone the array before mutating it, or don't zero it here.

src/main/java/org/opensearch/security/authtoken/jwt/EncryptionDecryptionUtil.java [69-71]

 public EncryptionDecryptionUtil(final byte[] secretBytes) {
-    this.aesKey = deriveKey(secretBytes);
+    this.aesKey = deriveKey(secretBytes.clone());
 }
Suggestion importance[1-10]: 8

__

Why: Valid concern: deriveKey zeroes secretBytes in its finally block, and fromSettings passes keystoreKey.getEncoded() which for some KeyStore implementations may return a shared reference, potentially corrupting the cached key. Cloning is a defensive improvement.

Medium
Avoid catching Throwable in check method

**Catching Throwable here silently swallows critical errors such as OutOfMemoryError,
ThreadDeath, and JVM-level Errors, which should never be caught and returned as
false. This can also mask FIPS configuration/provider issues that should surface.
Catch Exception (or...

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f908586

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit de757a3

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b9f7617

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b4fce9b

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8e3f614

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5c719f9

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b194efc

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 07f75d1

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit ab97aab

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 905dbb1

@beanuwave beanuwave changed the title make test-suite runnable under FIPS Run the Security plugin under FIPS approved-only mode with BC providers Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8f6e1e1

public static final String ADMIN_USER_NAME = "admin";
public static final String REGULAR_USER_NAME = "regular_user";
public static final String DEFAULT_PASSWORD = "secret";
private static final String encryptionKey = Base64.getEncoder().encodeToString("encryptionKey!!".getBytes(StandardCharsets.UTF_8));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"encryptionKey!!" at 120 bits satisfies cryptographic minimum the provider cares about >= 112bit. But we could reuse TestSecurityConfig.DEFAULT_TEST_PASSWORD here - so it future-proof.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to reuse, thanks

final String requestPath = "/*/_search";
final int parallelism = 20;
final int totalNumberOfRequests = 10_000;
final int parallelism = FipsMode.isEnabled() ? 8 : 20;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks odd, why do we have to change these settings? No relation to FIPS I believe

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generic client fires all requests at once ParallelFlux.from(monos) - for the test with 15k unbounded requests in FIPS-mode (executed in local env) ~80% of exceptions are connect/handshake timeouts at the node: the server can't accept() + handshake fast enough, so SYNs/handshakes expire. FIPS trips it first because the BCTLS handshake is ~x4 costlier.

IMO it comes down to two solutions:

  • Explicitly limit parallel requests - this code (introduces in fd9ed30)
  • Proper saturation via backpressure (parallelism = demand). Let Reactor apply backpressure so parallelism is the concurrency limit:
Flux.fromArray(monos).flatMap(mono -> mono, parallelism).collectList().block(...);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about third option to increase connect timeouts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generic-client storm (15k requests, parallelism 100, FIPS)

Metric 30s / 30s 120s / 120s
ok 7,920 (52.8%) 13,525 (90.2%)
failed 7,080 1,475
TCP_CONNECT_TIMEOUT 5,144 0
TLS_HANDSHAKE_TIMEOUT 839 0
TCP_CONNECTION_RESET 738 1,281
PREMATURE_CLOSE 359 194
elapsed time 50.1s 81.9s

Takeaway: raising the connect/handshake timeout (30s -> 120s) eliminates the*_TIMEOUT failures but cannot make the run green - TCP_CONNECTION_RESET (accept-queue overflow) grows and wall-clock rises.
2nd option is a better solution, because it keeps the accept queue shallow so nothing overflows and nothing waits near a timeout.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you, let's do 2nd option

if (protocol == HttpProtocol.HTTP11) {
Consumer<SslContextBuilder> http11Configure = s -> {
if (FipsMode.isEnabled()) {
s.sslProvider(SslProvider.JDK);

@reta reta Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we used to have openssl tests (and dependencies) but not anymore, how come non-JDK provider could be selected? We shouldn't have any on classpath

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, it's dead code on arrival. We can revert this change or replace it with assert !OpenSsl.isAvailable()


private static String fipsCompatibleEncryptionKey() {
final byte[] keyMaterial = new byte[32];
new SecureRandom().nextBytes(keyMaterial);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
new SecureRandom().nextBytes(keyMaterial);
Randomness.createSecure().nextBytes(keyMaterial)
```?


static {
byte[] bytes = new byte[16]; // satisfies BC FIPS 112-bit minimum
new SecureRandom().nextBytes(bytes);

@reta reta Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
new SecureRandom().nextBytes(bytes);
Randomness.createSecure().nextBytes(bytes);
```?

* <p>This is necessary because JNDI LDAP resolves hostnames to IP addresses before creating
* SSL sockets, making the hostname unavailable for SNI configuration.
*/
public class HostnameAwareConnectionFactory extends DefaultConnectionFactory {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LDAP related changes (SNI, etc) seems to be unrelated to FIPS. could we extract them into separate feature + pull request?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't quite call the LDAP changes FIPS-unrelated. The SNI plumbing and the PKCS12 workaround only become necessary because of running LDAPS through the BCTLS/BCFIPS provider stack.

That said, I do agree they're a fairly substantial, self-contained part of this PR. Splitting them out would make this diff smaller and keep LDAP-related chunk in one place.

Btw. as mentioned in the PR description, updating ldaptive for version 2.x would likely make the SNI plumbing unnecessary anyway.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @beanuwave

That said, I do agree they're a fairly substantial, self-contained part of this PR. Splitting them out would make this diff smaller and keep LDAP-related chunk in one place.

👍 , this is exactly the reasons

Btw. as mentioned in the PR description, updating ldaptive for version 2.x would likely make the SNI plumbing unnecessary anyway.

If we could do that (as separate change) so we could get rid of LDAP workarounds - even better

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sound like a plan - I'll prepare a new LDAP PR!

* @return true when the key material is held in a PKCS#11 token. Such keys are non-exportable, so the
* TLS engine must delegate signing to the token's provider (SunPKCS11) rather than BouncyCastle FIPS.
*/
default boolean isPkcs11() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we instead of checking PKCS11 in many places as true/false, represent that as data classes? For Pkcs11KeyStoreConfiguration and BcfksKeyStoreConfiguration fe?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly, this is a much cleaner design! Originally I rushed to get this through with a low-footprint design. However using polymorphism introduces quite a few code changes. Would you prefer the TrustStoreConfiguration implementations to remain as inner classes?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @beanuwave

Would you prefer the TrustStoreConfiguration implementations to remain as inner classes?

Yeah, I think we could keep it there (and may be make them sealed if that makes sense). thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3ac26d1 wdyt

@reta

reta commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

@beanuwave this is tremendous effort, thank you, a few high level comments:

  • the change is very large and difficult to review (it took me at least full day, split over the week), I strongly believe it will help to split it into smaller chunks (fe LDAP we already identified, there are probably other subareas)
  • the logic is primarily driven by ifs (fips mode, store types, etc..), if we could project that to data classes / abstractions, it will be much simpler to read and reason about
  • the upgrade path (for existing clusters) in unclear (to me): could non FIPS node be updated in place? could FIPS node join non-FIPS cluster? (or vice versa)

Hope it make sense, thank you!

@beanuwave

Copy link
Copy Markdown
Contributor Author

@reta Thank you for pushing this through the review process - that's great!

If you plan to do a second review, I'd really appreciate it if you could also take the "Reviewer call-outs" into account.

the logic is primarily driven by ifs (fips mode, store types, etc..), if we could project that to data classes / abstractions, it will be much simpler to read and reason about

The FIPS test class abstraction is a no-brainer, since it would then follow the same style as the core project. However, I'm not sure the same general rules can be applied to production code - it's more of a case-by-case assessment.

the upgrade path (for existing clusters) in unclear (to me): could non FIPS node be updated in place? could FIPS node join non-FIPS cluster? (or vice versa)

Do you have a specific scenario in mind where a heterogeneous cluster would be advantageous and provide real-world value? Assuming it would be possible for a non-FIPS node to join a FIPS cluster, IMO that would introduce a weak link in the chain and undermine the FIPS security guarantees of the entire cluster.

@reta

reta commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

If you plan to do a second review, I'd really appreciate it if you could also take the "Reviewer call-outs" into account.

👍

Do you have a specific scenario in mind where a heterogeneous cluster would be advantageous and provide real-world value? Assuming it would be possible for a non-FIPS node to join a FIPS cluster, IMO that would introduce a weak link in the chain and undermine the FIPS security guarantees of the entire cluster.

It is probably could be summarized like that: how would we recommend to start with FIPS support. Only brand new clusters or gradual migrations.

The FIPS test class abstraction is a no-brainer, since it would then follow the same style as the core project. However, I'm not sure the same general rules can be applied to production code - it's more of a case-by-case assessment

Correct, tests are indeed no-brainer, the one which really clicked for me for PCKS11, I think we could have cleaner implementation, but certainly - case by case.

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b5760ff

…th a StorePassword wrapper

Signed-off-by: Iwan Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 3ac26d1

import org.opensearch.security.support.PemKeyReader;

public interface KeyStoreConfiguration {
public sealed interface KeyStoreConfiguration {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

.endpointIdentificationAlgorithm(null)
.build();
.endpointIdentificationAlgorithm(null);
routePkcs11ThroughSunJsse(builder);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be add a method configure to KeyStoreConfiguration (noop for JDK, routePkcs11ThroughSunJsse for PKCS11): keyStoreConfiguration.configure(SslContextBuilder builder)

final var sslConfigSettings = settings.getByPrefix(fullSslConfigSuffix);
if (settings.hasValue(sslConfigSuffix + KEYSTORE_FILEPATH)) {
final var keyStorePassword = resolvePassword(sslConfigSuffix + KEYSTORE_PASSWORD, settings, DEFAULT_STORE_PASSWORD);
final boolean isPkcs11Keystore = PemKeyReader.PKCS11.equalsIgnoreCase(environment.settings().get(sslConfigSuffix + KEYSTORE_TYPE));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we moved those to factory methods? TrustStoreConfiguration::buildTrustStoreConfiguration and KeyStoreConfiguration::buildKeyStoreConfiguration ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC] FIPS-140 Compliance Roadmap for OpenSearch

5 participants